Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Always set the deployment target when building std #133092

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Always set the deployment target when building std #133092

wants to merge 4 commits into from
+118 −58

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Nov 16, 2024 •
edited
Loading

cc has a bug/feature (I guess depending on how you look at it) where the default deployment target is taken from the SDK instead of from rustc. This causes compiler-builtins to build compiler-rt with the wrong deployment target on iOS.

I've been meaning to change how cc works in this regard, but that's a lengthy process, so let's fix it in bootstrap for now.

The behaviour be seen locally with ./x build library --set build.optimized-compiler-builtins=true for various target triples, and then inspecting with otool -l build/host/stage1/lib/rustlib/*/lib/libcompiler_builtins-*.rlib | rg 'minos|version'. I have added a rmake test that ensures that we now have the same version everywhere.

Fixes #128419
Fixes rust-lang/compiler-builtins#650
See also rust-lang/cargo#13115

@rustbot label O-apple

@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) labels Nov 16, 2024
This was referenced Nov 16, 2024
Merged
Open
@madsmtm madsmtm force-pushed the bootstrap-deployment-target branch from fd382a0 to dd8ca01 Compare November 16, 2024 05:34
@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 16, 2024 •
edited
Loading

Okay... So this didn't actually work initially, since bootstrap is for some godforsaken reason calling cc to get default flags, and then later passing passing them to Cargo, which invokes the build script, which calls cc again to get default flags... So all the build flags end up getting duplicated (!), and the values in CFLAGS that were looked up previously end up being prioritized.

I've rebased and fixed that in the first and second commits, but it may break more than I think, so this should probably get a try build before merging.

@madsmtm
Bootstrap: Don't duplicate cc-rs flags
25e13fd 
Commands that end up invoking cc-rs, i.e. Cargo (through build scripts)
and cmake-rs don't need the CFLAGS from cc-rs itself, as they will just
end up as duplicates.
@madsmtm madsmtm force-pushed the bootstrap-deployment-target branch from dd8ca01 to 9ae3190 Compare November 16, 2024 06:13
@madsmtm
Remove outdated workaround for -mmacosx-version-min=
8729ba3 
This didn't actually activate properly, since cmake-rs calls cc-rs
internally, and as such the flag was set anyhow.
@madsmtm madsmtm force-pushed the bootstrap-deployment-target branch 2 times, most recently from 50812ff to 2294b05 Compare November 16, 2024 06:55
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Nov 16, 2024
@madsmtm madsmtm force-pushed the bootstrap-deployment-target branch from 2294b05 to f816d33 Compare November 16, 2024 07:05
@madsmtm madsmtm marked this pull request as ready for review November 16, 2024 07:19
@rustbot
Copy link
Collaborator

rustbot commented Nov 16, 2024

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 16, 2024

This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp.

Not sure if I should? It changes in theory (we pass different flags), but in practice it shouldn't (because those flags were already set by cmake-rs, so they were just duplicated)?

@Mark-Simulacrum
Copy link
Member

r=me modulo comment

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 28, 2024

📌 Commit ff3dab4 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 28, 2024
Zalathar added a commit to Zalathar/rust that referenced this pull request Nov 29, 2024
@Zalathar
Rollup merge of rust-lang#133092 - madsmtm:bootstrap-deployment-targe…
a8b54d6 
…t, r=Mark-Simulacrum

Always set the deployment target when building std

`cc` has [a bug/feature](rust-lang/cc-rs#1171) (I guess depending on how you look at it) where the default deployment target is taken from the SDK instead of from `rustc`. This causes `compiler-builtins` to build `compiler-rt` with the wrong deployment target on iOS.

I've been meaning to change how `cc` works in this regard, but that's a lengthy process, so let's fix it in bootstrap for now.

The behaviour be seen locally with `./x build library --set build.optimized-compiler-builtins=true` for various target triples, and then inspecting with `otool -l build/host/stage1/lib/rustlib/*/lib/libcompiler_builtins-*.rlib | rg 'minos|version'`. I have added a rmake test that ensures that we now have the same version everywhere.

Fixes rust-lang#128419
Fixes rust-lang/compiler-builtins#650
See also rust-lang/cargo#13115

`@rustbot` label O-apple
@Zalathar Zalathar mentioned this pull request Nov 29, 2024
Closed
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 29, 2024
@bors
Auto merge of rust-lang#133609 - Zalathar:rollup-1825fa8, r=Zalathar
9e6d698 
Rollup of 7 pull requests

Successful merges:

 - rust-lang#131323 (Support `clobber_abi` in AVR inline assembly)
 - rust-lang#133092 (Always set the deployment target when building std)
 - rust-lang#133134 (Don't use a SyntheticProvider for literally every type)
 - rust-lang#133538 (Better diagnostic for fn items in variadic functions)
 - rust-lang#133590 (Rename `-Zparse-only`)
 - rust-lang#133592 (Misc: better instructions for envrc, ignore `/build` instead of `build/`)
 - rust-lang#133608 (Revert rust-lang#133418 (Store coverage source regions as `Span`) due to regression rust-lang#133606)

r? `@ghost`
`@rustbot` modify labels: rollup
@Zalathar
Copy link
Contributor

Could this have caused the failure at #133609 (comment)?

@jieyouxu
Copy link
Member

This looks most likely I agree.
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 29, 2024
@bors
Copy link
Contributor

bors commented Dec 4, 2024

☔ The latest upstream changes (presumably #133841) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 10, 2024

☔ The latest upstream changes (presumably #134096) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-run-make Area: port run-make Makefiles to rmake.rs O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"was built for iOS 16.4" warning after updating from 1.79 to 1.80 "was built for iOS 16.4" warning
6 participants
@madsmtm @rustbot @Mark-Simulacrum @bors @Zalathar @jieyouxu